Add policy client and protocol plumbing by dsa0x · Pull Request #38514 · hashicorp/terraform

dsa0x · 2026-05-04T13:59:28Z

This is part of a stacked series to upstream the policy work in smaller, reviewable pieces:

This PR introduces the policy package and the basic protocol/client plumbing that the rest of the stack builds on.
The goal here is to land the policy-specific infrastructure first so the later PRs can focus on runtime behavior.

Included here

policy client setup and implementation
RPC protocol types,
policy diagnostics/types
callback service plumbing
generated protobufs
protobuf compile tool updates

Target Release

1.16.x

Rollback Plan

If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

This change is user-facing and I added a changelog entry.
This change is not user-facing.

SarahFrench

👋🏻 I've been taking a look at this today and will resume tomorrow, but here are some initial comments that mainly revolve around testing.

SarahFrench · 2026-05-18T16:03:38Z

+}
+
+func RegisterCallbackServiceServer(s grpc.ServiceRegistrar, srv CallbackServiceServer) {
+	// If the following call pancis, it indicates UnimplementedCallbackServiceServer was


Suggested change

// If the following call pancis, it indicates UnimplementedCallbackServiceServer was

// If the following call panics, it indicates UnimplementedCallbackServiceServer was

SarahFrench · 2026-05-18T16:43:37Z

+	}
+}
+
+func (c *client) Evaluate(ctx context.Context, req EvaluationRequest[*proto.ResourceMetadata]) EvaluationResponse {


Given the existence of EvaluateModule and EvaluateProvider, what do you think about renaming this EvaluateResource? I think this would make the code resemble the proto definition, too.

SarahFrench · 2026-05-18T17:24:02Z

+	return s.evaluateModuleFn(req)
+}
+
+func TestClientEvaluate(t *testing.T) {


I think there can be more test coverages here, both for this and the other 2 Evaluate-type methods.

For example:

You could have the evaluateResourceFn return an empty or nil response alongside a non-nil error - does the response from Evaluate handle that error as expected?

You could also include more data in the response from evaluateResourceFn and make more assertions about how it's transformed by the Evaluate method. E.g. if you include some PolicyDetails data and assert that diagnostics from in there are transformed as expected into diagnostics in the response from Evaluate.

SarahFrench · 2026-05-18T17:38:17Z

+	return diags
+}
+
+func (diagnostic *Diagnostic) ToHCL() *hcl.Diagnostic {


Could this benefit from some test coverage?

I know the concept of 'diagnostic extras' isn't new to the codebase, but this implementation is and it might be worth adding some light tests asserting that these behave in the way we expect.

➕ to that idea, I was also looking at the proto definition of diagnostic and it's a little difficult to understand exactly what data we will receive in a diagnostic. Perhaps some tests could serve as documentation of "these are some examples of what this extra info will look like"

SarahFrench · 2026-05-18T17:56:37Z

+				}, nil
+			},
+		},
+		callbackRegistry: callback.NewRegistry(),


Idea: could you make a mock callback registry to use in tests like this?

austinvalle

Very nice 👍🏻, I appreciate you breaking these out 😆. I left some comments and questions!

austinvalle · 2026-05-18T18:56:36Z

+
+message GetResourcesRequest {
+  string type = 1;
+  bytes data = 2;


nit: This is a little generic for the name, I believe the data is a filter object so perhaps filter would be more descriptive? 😅

austinvalle · 2026-05-18T18:57:38Z

+
+message GetDataSourceRequest {
+  string type = 1;
+  bytes data = 2;


nit: similar to the above, maybe this could be config?

austinvalle · 2026-05-18T18:59:46Z

+  uint32 evaluation_request_id = 3;
+}
+
+message GetResourcesResponse {


Should we also return diagnostics from this?

austinvalle · 2026-05-18T19:00:05Z

+  uint32 evaluation_request_id = 3;
+}
+
+message GetDataSourceResponse {


similar to above, should we also return diagnostics from this?

austinvalle · 2026-05-18T19:03:54Z

+  message Step {
+    oneof step {
+      string attribute = 1;
+      bytes index = 2;


is this a cty value that could be a string or number? 👀

austinvalle · 2026-05-18T20:40:13Z

+  // module_source is the source for the module that is used (e.g., "./modules/s3_bucket")
+  string module_source = 1;


nit: Is this needed since the source is also defined in ModuleMetadata? (or perhaps it can be removed from that shared type)

austinvalle · 2026-05-18T20:42:20Z

+  string source = 1;
+  string version = 2;


I'm assuming source and version are guaranteed to be evaluated at this point? So we don't need to worry about the recent dynamic module sourcing work impacting this 👀

austinvalle · 2026-05-18T20:43:43Z

+	default:
+		panic(fmt.Errorf("unsupported Step type: %T", step))


nit: Does this method need an error return if we're going to panic here anyways 👀 (similar question to the above ToCtyPath)

austinvalle · 2026-05-18T20:59:13Z

+		Start:    rng.Start.ToHclPos(),
+		End:      rng.End.ToHclPos(),


Are these guaranteed to be non-nil? 👀

austinvalle · 2026-05-18T21:06:39Z

+		// should be exhaustive
+		panic("unhandled EvaluateResult")


Suggested change

// should be exhaustive

panic("unhandled EvaluateResult")

// should be exhaustive

panic(fmt.Errorf("unhandled EvaluateResult type: %T", result))

Add policy client and protocol plumbing

e0c86a2

dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label May 4, 2026

This was referenced May 5, 2026

Add policy result types and diagnostic metadata plumbing #38515

Open

Wire policy evaluation into Terraform core runtime #38516

Open

Add CLI and backend policy wiring for plan apply and query #38518

Draft

Add init-time policy evaluation #38519

Draft

dsa0x marked this pull request as ready for review May 18, 2026 09:52

dsa0x requested a review from a team as a code owner May 18, 2026 09:52

SarahFrench reviewed May 18, 2026

View reviewed changes

austinvalle reviewed May 18, 2026

View reviewed changes

	// If the following call pancis, it indicates UnimplementedCallbackServiceServer was
	// If the following call panics, it indicates UnimplementedCallbackServiceServer was

		// module_source is the source for the module that is used (e.g., "./modules/s3_bucket")
		string module_source = 1;

		default:
		panic(fmt.Errorf("unsupported Step type: %T", step))

Conversation

dsa0x commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Included here

Target Release

Rollback Plan

Changes to Security Controls

CHANGELOG entry

Uh oh!

SarahFrench left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dsa0x commented May 4, 2026 •

edited

Loading